-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: improve-two-factor-authentication-features #377
Automated Test: improve-two-factor-authentication-features #377
Conversation
Co-authored-by: Peer Richelsen <peeroke@gmail.com>
📝 WalkthroughWalkthroughThis PR introduces backup codes as an alternative two-factor authentication method. Changes include a new backup code input component, backup code generation during 2FA setup, backup code validation during login and 2FA disable, database schema extensions for encrypted storage, updated authentication flows, new error codes, and comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Browser as Browser / UI
participant Server as Server / API
participant DB as Database
rect rgba(100, 150, 255, 0.5)
Note over User,DB: Backup Code Generation During 2FA Setup
User->>Browser: Initiate 2FA setup
Browser->>Server: POST /setup with TOTP data
Server->>Server: Generate 10 backup codes
Server->>Server: Encrypt backup codes
Server->>DB: Store encrypted backupCodes
Server->>Browser: Return backupCodes + QR code
Browser->>User: Display backup codes (copy/download/close)
User->>Browser: Copy or download backup codes
end
rect rgba(150, 200, 100, 0.5)
Note over User,DB: Backup Code Validation During Login
User->>Browser: Enter backup code (lost access path)
Browser->>Server: POST /login with backupCode
Server->>DB: Fetch user backupCodes
Server->>Server: Decrypt backupCodes
Server->>Server: Validate code against decrypted codes
alt Code matches
Server->>Server: Remove used code from array
Server->>Server: Re-encrypt remaining codes
Server->>DB: Update backupCodes with remaining codes
Server->>Browser: Authentication successful
else Code invalid
Server->>Browser: Return IncorrectBackupCode error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/playwright/login.2fa.e2e.ts (1)
44-55:⚠️ Potential issue | 🟠 MajorFirst test's enable-2FA step is missing backup codes handling — will likely hang.
The second test ("should allow a user to disable 2FA") handles the backup codes dialog after
fillOtp(lines 110–125: download, copy, close). This test does not. If the UI now shows the backup codes dialog after enabling 2FA, this test will hang waiting for the switch selector on line 52 because the dialog is blocking interaction.Add the same backup codes dismiss flow here, or at minimum close the dialog before asserting the switch state.
Additionally,
page.locator(...).isChecked()on line 53 returns aPromise<boolean>, soexpect(...).toBeTruthy()always passes (a Promise object is truthy). The FIXME comment acknowledges this, but it should be fixed as well — the second test correctly usestoBeVisible()withdata-state="checked"(line 127).Proposed fix
await fillOtp({ page, // eslint-disable-next-line `@typescript-eslint/no-non-null-assertion` secret: secret!, }); - // FIXME: this passes even when switch is not checked, compare to test - // below which checks for data-state="checked" and works as expected - await page.waitForSelector(`[data-testid=two-factor-switch]`); - await expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy(); + // backup codes are now showing, so dismiss them + const promise = page.waitForEvent("download"); + await page.getByTestId("backup-codes-download").click(); + const download = await promise; + expect(download.suggestedFilename()).toBe("cal-backup-codes.txt"); + + await page.getByTestId("backup-codes-copy").click(); + await page.getByTestId("toast-success").waitFor(); + + await page.getByTestId("backup-codes-close").click(); + + await expect(page.locator(`[data-testid=two-factor-switch][data-state="checked"]`)).toBeVisible();
🤖 Fix all issues with AI agents
In `@apps/web/components/auth/BackupCode.tsx`:
- Line 7: Rename the default exported React component function from TwoFactor to
BackupCode in BackupCode.tsx so the component name matches the file and imports;
update the function declaration (currently "function TwoFactor({ center = true
})") to "function BackupCode(...)" while preserving props like center and the
existing default export statement so references and debugging align with the
actual component name.
In `@apps/web/components/settings/EnableTwoFactorModal.tsx`:
- Around line 71-75: The blob URL created with URL.createObjectURL in the
EnableTwoFactorModal is never revoked, causing a memory leak; add a useEffect
inside the component that tracks the generated blob URL (the variable created
near the QR/image creation and used in the modal) and in the cleanup function
call URL.revokeObjectURL(blobUrl) when the component unmounts or when a new blob
URL is generated, and also ensure any close/reset logic (e.g., resetState)
clears/revokes the current blob URL before clearing state.
- Around line 64-65: The backupCodes state is inferred as never[] because
useState([]) has no type parameter; update the declaration for backupCodes to
include a concrete array type (e.g., useState<string[]>() or the correct item
type) so setBackupCodes(body.backupCodes) and backupCodes.map(...) are
type-safe; locate the useState call for backupCodes and change it to
useState<string[]>([]) (or the appropriate interface) and ensure any API
response assigned to setBackupCodes matches that type.
- Around line 283-287: The download Button inside EnableTwoFactorModal is
rendered within a Form (and so defaults to type="submit"), causing handleEnable
to be invoked when users click "Download"; update the Button in
EnableTwoFactorModal.tsx (the <Button> wrapped by the <a download={...}>) to
include type="button" (or add an onClick that calls e.preventDefault()) so
clicking Download does not submit the form or re-run handleEnable.
- Around line 273-282: The onClick handler for the Button in
EnableTwoFactorModal currently calls navigator.clipboard.writeText without
awaiting or handling rejection, so showToast is fired even if copy fails; update
the Button's onClick (the inline handler in EnableTwoFactorModal) to await
navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n"))
inside a try/catch and call showToast(t("backup_codes_copied"), "success") only
on success, and call showToast with an error message on catch (handle
permission/denial errors gracefully).
In `@packages/features/auth/lib/next-auth-options.ts`:
- Around line 131-157: Wrap the JSON.parse(symmetricDecrypt(...)) call that
reads user.backupCodes in a try/catch and on any parse/decrypt error throw
Error(ErrorCode.InternalServerError) to avoid unhandled exceptions; additionally
prevent the password-bypass by rejecting requests that supply both
credentials.totpCode and credentials.backupCode (e.g., check if both are present
and throw Error(ErrorCode.BadRequest)) or alternatively ensure the password
verification path runs unconditionally when twoFactorEnabled — update the logic
around user.twoFactorEnabled, credentials.totpCode and credentials.backupCode to
implement one of these fixes and adjust subsequent backup code handling
(backupCodes array processing, symmetricEncrypt, prisma.user.update)
accordingly.
🧹 Nitpick comments (10)
packages/ui/components/form/inputs/Input.tsx (1)
47-51: Minor accessibility trade-off withtabIndex={-1}.Adding
tabIndex={-1}removes the password visibility toggle from the tab order. This simplifies form navigation but means keyboard-only users cannot toggle password visibility without a mouse. This is a common pattern, but worth noting that it reduces keyboard accessibility.apps/web/pages/api/auth/two-factor/totp/setup.ts (1)
60-61: Consider hashing backup codes instead of encrypting them.Backup codes only need to be verified, not decrypted. Hashing each code individually (e.g., with
bcryptorscrypt) would provide stronger security — if the encryption key is compromised, encrypted backup codes are immediately exposed, whereas hashed codes remain protected.That said, this follows the same symmetric encryption pattern already used for
twoFactorSecretin this codebase, so it's consistent.apps/web/components/settings/TwoFactorAuthAPI.ts (1)
22-25: Consider makingbackupCodeoptional.The UI is mutually exclusive—users provide either a TOTP code or a backup code, not both. Currently, one value is always an empty string. Making
backupCodeoptional and conditionally including it in the request body would better express intent and avoid unnecessary empty parameters:♻️ Suggested change
- async disable(password: string, code: string, backupCode: string) { + async disable(password: string, code: string, backupCode?: string) { return fetch("/api/auth/two-factor/totp/disable", { method: "POST", - body: JSON.stringify({ password, code, backupCode }), + body: JSON.stringify({ password, code, ...(backupCode && { backupCode }) }),apps/web/playwright/login.2fa.e2e.ts (1)
12-13: TODO noted: add backup code login and disable tests.The comment on line 12 calls out missing test scenarios for logging in with a backup code and disabling 2FA with a backup code. These are important paths given the new feature. Consider tracking this as a follow-up issue.
Would you like me to open a GitHub issue to track the missing backup-code test scenarios (login with backup code, disable 2FA with backup code)?
apps/web/pages/api/auth/two-factor/totp/disable.ts (1)
46-64: Duplicated backup-code validation logic — extract a shared helper.The decrypt → parse → indexOf → invalidate flow here (lines 48–64) is nearly identical to the one in
next-auth-options.ts(lines 131–156). Both decrypt backup codes, strip dashes, look up the code, and handle the same error cases. This duplication means any future fix (e.g., addingJSON.parseerror handling) must be applied in both places.Consider extracting a shared utility, e.g.,
validateBackupCode(encryptedCodes, suppliedCode, encryptionKey), that both call sites can use.Also,
JSON.parseon line 58 will throw an unhandled exception if the decrypted payload is malformed — same concern flagged innext-auth-options.ts.apps/web/pages/auth/login.tsx (2)
73-80: Backup code errors handled outside theerrorMessagesmap — inconsistent pattern.
IncorrectBackupCodeandMissingBackupCodesare handled inline (lines 157–158) while all other credential errors go through theerrorMessagesobject (lines 73–80). This makes the error handling less discoverable.Proposed refactor
const errorMessages: { [key: string]: string } = { // [ErrorCode.SecondFactorRequired]: t("2fa_enabled_instructions"), // Don't leak information about whether an email is registered or not [ErrorCode.IncorrectEmailPassword]: t("incorrect_email_password"), [ErrorCode.IncorrectTwoFactorCode]: `${t("incorrect_2fa_code")} ${t("please_try_again")}`, [ErrorCode.InternalServerError]: `${t("something_went_wrong")} ${t("please_try_again_and_contact_us")}`, [ErrorCode.ThirdPartyIdentityProviderEnabled]: t("account_created_with_identity_provider"), + [ErrorCode.IncorrectBackupCode]: t("incorrect_backup_code"), + [ErrorCode.MissingBackupCodes]: t("missing_backup_codes"), };Then remove the dedicated
else ifbranches on lines 157–158 — the fallback on line 160 (errorMessages[res.error]) will handle them.Also applies to: 157-158
221-221: Nested ternary is hard to scan.This line has two nested ternaries. Consider restructuring for readability, though it's functionally correct.
Suggested alternative
- {twoFactorRequired ? !twoFactorLostAccess ? <TwoFactor center /> : <BackupCode center /> : null} + {twoFactorRequired && ( + twoFactorLostAccess ? <BackupCode center /> : <TwoFactor center /> + )}apps/web/components/settings/EnableTwoFactorModal.tsx (3)
71-75:resetStateshould clear sensitive data (backup codes) from state.When the user cancels or closes the modal,
resetStateresetspassword,errorMessage, andstepbut leavesbackupCodesandbackupCodesUrlin memory. Since backup codes are security-sensitive, clear them on reset to minimize exposure.Proposed fix
const resetState = () => { setPassword(""); setErrorMessage(null); setStep(SetupStep.ConfirmPassword); + setBackupCodes([]); + if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl); + setBackupCodesUrl(""); };
163-163: Nit:formatBackupCodeis a pure function — extract outside the component.It has no dependency on component state or props, so it's recreated every render unnecessarily.
Proposed refactor
Move the function definition above the component:
+const formatBackupCode = (code: string) => `${code.slice(0, 5)}-${code.slice(5, 10)}`; + const EnableTwoFactorModal = ({ onEnable, onCancel, open, onOpenChange }: EnableTwoFactorModalProps) => { ... - const formatBackupCode = (code: string) => `${code.slice(0, 5)}-${code.slice(5, 10)}`;
174-182: Nit:type="password"is redundant onPasswordField.
PasswordFieldtypically renders a password input by default. The explicittype="password"prop is harmless but unnecessary.
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | ||
| import { Label, TextField } from "@calcom/ui"; | ||
|
|
||
| export default function TwoFactor({ center = true }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Rename the function from TwoFactor to BackupCode to match the file name.
The default export is named TwoFactor, but this file is BackupCode.tsx and it's imported as BackupCode elsewhere. The name TwoFactor is already used by the actual TOTP component in TwoFactor.tsx, making this confusing during debugging and code navigation.
Proposed fix
-export default function TwoFactor({ center = true }) {
+export default function BackupCode({ center = true }) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function TwoFactor({ center = true }) { | |
| export default function BackupCode({ center = true }) { |
🤖 Prompt for AI Agents
In `@apps/web/components/auth/BackupCode.tsx` at line 7, Rename the default
exported React component function from TwoFactor to BackupCode in BackupCode.tsx
so the component name matches the file and imports; update the function
declaration (currently "function TwoFactor({ center = true })") to "function
BackupCode(...)" while preserving props like center and the existing default
export statement so references and debugging align with the actual component
name.
| const [backupCodes, setBackupCodes] = useState([]); | ||
| const [backupCodesUrl, setBackupCodesUrl] = useState(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backupCodes state is typed as never[] — add explicit type annotation.
useState([]) without a type parameter infers never[], which means setBackupCodes(body.backupCodes) on Line 92 and backupCodes.map(...) on Line 203 will produce TypeScript errors (or silently fail if any leaks in from the API response).
Proposed fix
- const [backupCodes, setBackupCodes] = useState([]);
+ const [backupCodes, setBackupCodes] = useState<string[]>([]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [backupCodes, setBackupCodes] = useState([]); | |
| const [backupCodesUrl, setBackupCodesUrl] = useState(""); | |
| const [backupCodes, setBackupCodes] = useState<string[]>([]); | |
| const [backupCodesUrl, setBackupCodesUrl] = useState(""); |
🤖 Prompt for AI Agents
In `@apps/web/components/settings/EnableTwoFactorModal.tsx` around lines 64 - 65,
The backupCodes state is inferred as never[] because useState([]) has no type
parameter; update the declaration for backupCodes to include a concrete array
type (e.g., useState<string[]>() or the correct item type) so
setBackupCodes(body.backupCodes) and backupCodes.map(...) are type-safe; locate
the useState call for backupCodes and change it to useState<string[]>([]) (or
the appropriate interface) and ensure any API response assigned to
setBackupCodes matches that type.
| const resetState = () => { | ||
| setPassword(""); | ||
| setErrorMessage(null); | ||
| setStep(SetupStep.ConfirmPassword); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blob URL is never revoked on unmount — memory/resource leak.
URL.createObjectURL on Line 99 creates a blob URL that persists until explicitly revoked. If the modal unmounts (e.g., user navigates away) before the user clicks "Close", the blob URL is leaked. Add a cleanup effect.
Proposed fix — add useEffect cleanup
+ useEffect(() => {
+ return () => {
+ if (backupCodesUrl) URL.revokeObjectURL(backupCodesUrl);
+ };
+ }, [backupCodesUrl]);Also applies to: 92-99
🤖 Prompt for AI Agents
In `@apps/web/components/settings/EnableTwoFactorModal.tsx` around lines 71 - 75,
The blob URL created with URL.createObjectURL in the EnableTwoFactorModal is
never revoked, causing a memory leak; add a useEffect inside the component that
tracks the generated blob URL (the variable created near the QR/image creation
and used in the modal) and in the cleanup function call
URL.revokeObjectURL(blobUrl) when the component unmounts or when a new blob URL
is generated, and also ensure any close/reset logic (e.g., resetState)
clears/revokes the current blob URL before clearing state.
| <Button | ||
| color="secondary" | ||
| data-testid="backup-codes-copy" | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n")); | ||
| showToast(t("backup_codes_copied"), "success"); | ||
| }}> | ||
| {t("copy")} | ||
| </Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
navigator.clipboard.writeText is async — toast fires even if copy fails.
writeText returns a Promise that rejects if the clipboard permission is denied. The success toast will show regardless. Await the call and catch failures.
Proposed fix
<Button
color="secondary"
data-testid="backup-codes-copy"
- onClick={(e) => {
+ onClick={async (e) => {
e.preventDefault();
- navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n"));
- showToast(t("backup_codes_copied"), "success");
+ try {
+ await navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n"));
+ showToast(t("backup_codes_copied"), "success");
+ } catch {
+ showToast(t("something_went_wrong"), "error");
+ }
}}>🤖 Prompt for AI Agents
In `@apps/web/components/settings/EnableTwoFactorModal.tsx` around lines 273 -
282, The onClick handler for the Button in EnableTwoFactorModal currently calls
navigator.clipboard.writeText without awaiting or handling rejection, so
showToast is fired even if copy fails; update the Button's onClick (the inline
handler in EnableTwoFactorModal) to await
navigator.clipboard.writeText(backupCodes.map(formatBackupCode).join("\n"))
inside a try/catch and call showToast(t("backup_codes_copied"), "success") only
on success, and call showToast with an error message on catch (handle
permission/denial errors gracefully).
| <a download="cal-backup-codes.txt" href={backupCodesUrl}> | ||
| <Button color="primary" data-testid="backup-codes-download"> | ||
| {t("download")} | ||
| </Button> | ||
| </a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Download <Button> inside a <Form> may trigger form submission.
The <Button> within the <a download> tag has no explicit type prop. Inside the <Form>, it defaults to type="submit", which would invoke handleEnable again when the user clicks "Download". The Close and Copy buttons correctly call e.preventDefault(), but this one relies on the <a> wrapper alone, which doesn't prevent form submission.
Proposed fix — add type="button" or onClick with preventDefault
<a download="cal-backup-codes.txt" href={backupCodesUrl}>
- <Button color="primary" data-testid="backup-codes-download">
+ <Button color="primary" type="button" data-testid="backup-codes-download">
{t("download")}
</Button>
</a>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a download="cal-backup-codes.txt" href={backupCodesUrl}> | |
| <Button color="primary" data-testid="backup-codes-download"> | |
| {t("download")} | |
| </Button> | |
| </a> | |
| <a download="cal-backup-codes.txt" href={backupCodesUrl}> | |
| <Button color="primary" type="button" data-testid="backup-codes-download"> | |
| {t("download")} | |
| </Button> | |
| </a> |
🤖 Prompt for AI Agents
In `@apps/web/components/settings/EnableTwoFactorModal.tsx` around lines 283 -
287, The download Button inside EnableTwoFactorModal is rendered within a Form
(and so defaults to type="submit"), causing handleEnable to be invoked when
users click "Download"; update the Button in EnableTwoFactorModal.tsx (the
<Button> wrapped by the <a download={...}>) to include type="button" (or add an
onClick that calls e.preventDefault()) so clicking Download does not submit the
form or re-run handleEnable.
| if (user.twoFactorEnabled && credentials.backupCode) { | ||
| if (!process.env.CALENDSO_ENCRYPTION_KEY) { | ||
| console.error("Missing encryption key; cannot proceed with backup code login."); | ||
| throw new Error(ErrorCode.InternalServerError); | ||
| } | ||
|
|
||
| if (!user.backupCodes) throw new Error(ErrorCode.MissingBackupCodes); | ||
|
|
||
| const backupCodes = JSON.parse( | ||
| symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) | ||
| ); | ||
|
|
||
| // check if user-supplied code matches one | ||
| const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", "")); | ||
| if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode); | ||
|
|
||
| // delete verified backup code and re-encrypt remaining | ||
| backupCodes[index] = null; | ||
| await prisma.user.update({ | ||
| where: { | ||
| id: user.id, | ||
| }, | ||
| data: { | ||
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), | ||
| }, | ||
| }); | ||
| } else if (user.twoFactorEnabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backup code validation: unhandled JSON.parse failure and password-bypass edge case.
Two observations on this block:
-
Unhandled parse error: If
user.backupCodesdecrypts to malformed JSON,JSON.parseon line 139 will throw an unhandled exception, surfacing as a generic 500 to the caller. Consider wrapping in a try/catch that returnsInternalServerError. -
Password bypass when both
totpCodeandbackupCodeare supplied: The password check on line 121 is gated by!credentials.totpCode. If a caller provides bothtotpCodeandbackupCode, the password check is skipped and only the backup code is validated. In practice the UI sends one or the other, but a direct API caller could exploit this to skip password verification. Consider validating the password unconditionally or rejecting requests that supply both codes.
Proposed fix for JSON.parse safety
- const backupCodes = JSON.parse(
- symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)
- );
+ let backupCodes: string[];
+ try {
+ backupCodes = JSON.parse(
+ symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)
+ );
+ } catch {
+ console.error("Failed to decrypt/parse backup codes for user", user.id);
+ throw new Error(ErrorCode.InternalServerError);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (user.twoFactorEnabled && credentials.backupCode) { | |
| if (!process.env.CALENDSO_ENCRYPTION_KEY) { | |
| console.error("Missing encryption key; cannot proceed with backup code login."); | |
| throw new Error(ErrorCode.InternalServerError); | |
| } | |
| if (!user.backupCodes) throw new Error(ErrorCode.MissingBackupCodes); | |
| const backupCodes = JSON.parse( | |
| symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) | |
| ); | |
| // check if user-supplied code matches one | |
| const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", "")); | |
| if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode); | |
| // delete verified backup code and re-encrypt remaining | |
| backupCodes[index] = null; | |
| await prisma.user.update({ | |
| where: { | |
| id: user.id, | |
| }, | |
| data: { | |
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), | |
| }, | |
| }); | |
| } else if (user.twoFactorEnabled) { | |
| if (user.twoFactorEnabled && credentials.backupCode) { | |
| if (!process.env.CALENDSO_ENCRYPTION_KEY) { | |
| console.error("Missing encryption key; cannot proceed with backup code login."); | |
| throw new Error(ErrorCode.InternalServerError); | |
| } | |
| if (!user.backupCodes) throw new Error(ErrorCode.MissingBackupCodes); | |
| let backupCodes: string[]; | |
| try { | |
| backupCodes = JSON.parse( | |
| symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) | |
| ); | |
| } catch { | |
| console.error("Failed to decrypt/parse backup codes for user", user.id); | |
| throw new Error(ErrorCode.InternalServerError); | |
| } | |
| // check if user-supplied code matches one | |
| const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", "")); | |
| if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode); | |
| // delete verified backup code and re-encrypt remaining | |
| backupCodes[index] = null; | |
| await prisma.user.update({ | |
| where: { | |
| id: user.id, | |
| }, | |
| data: { | |
| backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), | |
| }, | |
| }); | |
| } else if (user.twoFactorEnabled) { |
🤖 Prompt for AI Agents
In `@packages/features/auth/lib/next-auth-options.ts` around lines 131 - 157, Wrap
the JSON.parse(symmetricDecrypt(...)) call that reads user.backupCodes in a
try/catch and on any parse/decrypt error throw
Error(ErrorCode.InternalServerError) to avoid unhandled exceptions; additionally
prevent the password-bypass by rejecting requests that supply both
credentials.totpCode and credentials.backupCode (e.g., check if both are present
and throw Error(ErrorCode.BadRequest)) or alternatively ensure the password
verification path runs unconditionally when twoFactorEnabled — update the logic
around user.twoFactorEnabled, credentials.totpCode and credentials.backupCode to
implement one of these fixes and adjust subsequent backup code handling
(backupCodes array processing, symmetricEncrypt, prisma.user.update)
accordingly.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Tests